Repository navigation
fix: redact decrypted config output from pod logs on config load failure - #40
Merged
Merged
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
![eyevinn-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/7300575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Research Review
Verified
- Premise: the only two prints of
config_env_output(old lines 245/261) are replaced;grep -n config_env_outputon the PR head shows the remaining uses are only the empty check, the export grep and the new name-only extraction. Directions match the architect triage on #38. - Helper placement:
redact_config_log()sits after the# Load environment variables from config servicecomment, inside the range the test's awk extracts, so it is defined under test. - Pinned CLI behaviour (stdout =
export KEY='value', stderr =err.message) is as stated in the triage (@osaas/cli@4.35.0); no new external API used (bash +sed -E). - Reproduced locally on head 025d5f6:
bash -nOK; all 7tests/test-entrypoint-*.shpass (config-load-failure 11/0, credential-header-auth 13/0, credential-scrub 6/0, fragment-branch 8/0, setup-timeout 7/0, subpath 7/0, commit-info-exclude all passed), matching the PR body.
Adjacent Stubs Detected
- None (no TODO/FIXME/HACK/
undefinedpatterns in the touched script).
Orphaned Files Detected
- None. Both touched files are entry scripts/tests invoked directly.
Escape Hatch Coverage
- No new terminal-rejection branch: exit codes (124,
config_exit, 1) and the fatal path are unchanged. Passing path remains covered (okcase, now also assertingquxis in neither stream).
Object Shape Verification
- No spreads of fetched objects (bash only).
Contract diff
- No request handler or timeout changed (checked
scripts/docker-entrypoint.sh,tests/test-entrypoint-config-load-failure.sh). Only stderr log text changes; theLoading environment variablesline,refresh-app-confighint text and exit codes are unchanged.
Impact Analysis
- Blast radius: python-runner pod log text on config-load failure only.
- Downstream consumers: osaas-ai (
instructions.tsrule 34,myapps.ts:4346) key on the unchanged Loading line and hint; audit per the triage and PR body, with the stated coverage limit (tenant scripts outside the searched repos). - Coordinated changes needed: none. Prod sync-fork/remake is birme's call, stated in the PR.
- Breaking change risk: low.
Risks Not Tested
- Tests are not wired to CI (no checks on the branch); I ran them locally only.
- Redaction is pattern-based; a secret in stderr that is not one of the four runner tokens, a
gh[pso]_token, a JWT or Basic credential would pass through. Stdout is never printed, so this only concerns CLI error messages. - Sibling runners php-runner and wasm-runner still print the same output; noted in the PR Lessons, out of scope here.
Research Suggestions
- Consider a follow-up ticket for php-runner and wasm-runner (the PR Lessons say none exists).
Verdict
No blocking issues found.
Reviewed by PR Research Validator. This review checks whether code is based on verified sources rather than assumptions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the leak of decrypted parameter-store values to pod logs when the
CONFIG_SVCload fails inscripts/docker-entrypoint.sh.redact_config_log()helper, defined inside the extracted config block (the test's awk range). It replaces the literal values ofOSC_ACCESS_TOKEN,CONFIG_API_KEY,GIT_TOKEN,GITHUB_TOKEN, thengh[pso]_tokens, JWT-shaped strings andBasiccredentials. Pure bash plussed -E, no new dependency.$config_err_outputonly;$config_env_output(the CLI stdout with decrypted values) is never printed.refresh-app-confighint now greps$config_err_outputonly, so a secret value containing401no longer triggers it.values not logged, never the values.Loading environment variablesline, the Timed out line, theeval, theset -ecomment, exit codes (124,config_exit, 1). The Low-severity items (root user,curl | bash,pip install gradio) are not touched.Consumer impact
Log text only. The
[CONFIG] Loading environment variables from config serviceline, therefresh-app-confighint text and all exit codes are unchanged. Audited by the triage in osaas-ai (instructions.tsrule 34,myapps.ts:4346), osaas-deploy-manager, osaas-app and osaas-e2e-tests: nothing parses the changed[CONFIG] ERRORlines. A consumer outside those repos (a tenant script reading its own logs) would not have been found.Tests
Not wired to CI; run locally from the repo root.
bash -n scripts/docker-entrypoint.sh: OKtests/test-entrypoint-config-load-failure.sh: 11 passed, 0 failed (7 existing plus new cases: secret on stdout during failure, invalid key name without its value, token/ghp/JWT redaction in stderr, stdout no longer drives the hint; theokcase also assertsquxis in neither stream).Other suites: commit-info-exclude: All tests passed; credential-header-auth: 13 passed, 0 failed; credential-scrub: 6 passed, 0 failed; fragment-branch: 8 passed, 0 failed; setup-timeout: 7 passed, 0 failed; subpath: 7 passed, 0 failed.
What a human does next:
A prod sync-fork and remake of
eyevinn-python-runneris birme's call; running apps pick the fix up on their next rebuild, and historical logs are not rewritten. The dev counterpart is agent-run after merge:.claude/scripts/dev-remake.sh https://github.com/Eyevinn/python-runner eyevinn-python-runner.Closes #38
Lessons
A redaction helper has to live inside the block the test extracts with awk, otherwise it is
command not foundunder test; and the realistic leak was not the failure branch but the "no valid export lines" branch, where a key likemy-keymade the whole decrypted stdout fail the grep and get echoed. Siblings php-runner and wasm-runner still print the same output and have no ticket.🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com